-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added doc for header and json format for pip show #8008
Conversation
0aaa554
to
78b42de
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this @deveshks.
I'm not sure we should couple the human readable output and the json output, and take the opportunity that json output is almost absent today to design it from a blank page. So I personally would not take the current output as a basis.
Since the human readable output is not documented today, it is maybe not worth the trouble documenting it, to not encourage tools authors to parse it. Of course we should not change it in case some people depend on it.
For the new json output, how about a structure with the following keys:
- metadata: a dictionary with the core metadata fields, lower cased, with dashes replaced by underscores (https://packaging.python.org/specifications/core-metadata/)
- direct_url: the content of direct_url.json, if present (PEP 610)
- installer: the content of INSTALLER if present (PEP 376)
- record: a list of {path, hash, size} from RECORD (PEP 376)
- requested: a boolean (true if REQUESTED is present) (PEP 376)
- required_by: a list of canonicalized distribution names that depend on this entry
- location: this probably requires some investigation (it's different if the install is editable or not)
Also do we need a flag for editables? Probably for legacy ediables. For modern editables (which generate .dist-info metadata), this is covered by direct_url.
Hi @sbidoul
Agreed that we can have a newer json output with different fields as you suggested. I have added the new keys according to your suggestion, and instead of expanding on each individual sub-keys, I have referred to the corresponding URL for PEP or packaging. Please take a look
Could you expand on what do you mean when you say I think that once we agree on the content of json output based on this document, I will resume my work on the code PR to add these fields, and the respective test cases. |
docs/html/reference/pip_show.rst
Outdated
- **metadata:** A dictionary with the core metadata fields present in the METADATA file, as defined in the | ||
`Core metadata specifications`_. The fields are lower cased, with dashes replaced by underscores. | ||
- **direct_url:** A dictionary containing the content of direct_url.json, if present, as specified in `PEP 610`_ | ||
- **installer:** A string containing the content of INSTALLER, if present, as specified in `PEP 376`_ | ||
- **record:** A list of {file_path, file_content_hash, file_size|, representing the content of RECORD, | ||
if present, as specified in `PEP 376`_ . | ||
- **requested:** A boolean, set to True if REQUESTED is present, as specified in `PEP 376`_ | ||
- **required_by:** A list of canonicalized distribution names that depend on the distribution in question | ||
- **location:** A string containing the path where the distribution is installed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I think definition list markup might fit this part better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @McSinyx
Did you mean something like this?
- **metadata:**
A dictionary with the core metadata fields present in the METADATA file, as defined in the
`Core metadata specifications`_. The fields are lower cased, with dashes replaced by underscores.
- **direct_url:**
A dictionary containing the content of direct_url.json, if present, as specified in `PEP 610`_
....
It looks like this when rendered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant something line this
Restructured Text
metadata
A dictionary with the core metadata fields present in the ``METADATA`` file,
as defined in the `Core metadata specifications`_. The fields are
lower cased, with dashes replaced by underscores.
direct_url
A dictionary containing the content of ``direct_url.json``,
if present, as specified in :pep`610`.
installer
A string containing the content of ``INSTALLER``,
if present, as specified in :pep:`376`.
record
A list of ``[file_path, file_content_hash, file_size]``, representing
the content of ``RECORD``, if present, as specified in :pep:`376`.
requested
A boolean, set to True if ``REQUESTED`` is present,
as specified in :pep:`376`.
required_by
A list of canonicalized distribution names that depend
on the queried distribution.
location
A string containing the path where the distribution is installed.
.. _`Core metadata specifications`: https://packaging.python.org/specifications/core-metadata/
I nit a few spots, e.g. typos and PEP references, though I have no strong opinion on the bold part.
Edit, seemingly field list would be a better choice if bold is desired. My point is to make the source easier to read.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @McSinyx ,
That looks much more readable than what I had. . I have italized the fieldnames instead of making them bold. Also TIL that :pep:`376`.
automatically creates the PEP link.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2c4c273
to
a0a0a5f
Compare
Seems like the travis-ci job timed out in the latest commit (https://travis-ci.org/github/pypa/pip/jobs/673344968#L276-L277) . I think it should probably get fixed in further commits to this PR. Edit: Closing and Reopening fixed the issue. All checks pass now. |
Noting that PEP 566 has a section explaining how to convert metadata to json. https://www.python.org/dev/peps/pep-0566/#json-compatible-metadata |
Thanks @sbidoul , So given this information and this PR where I have documented the metadata according to your suggestions, can I start implementing some of the parsing logic in the original PR #7967 ? Also if you have other suggestions about the document, please let me know the same, so that I can incorporate them to both the PRs 😊 |
A few topics to discuss and resolve:
|
Thanks for those topics. It seems like some of the topics will affect how a particular thing will be implemented, and how will it then be implemented. Also how do we generally drive such a discussion, as mentioned in the task list above in a PR? |
@sbidoul , Please find my opinions on the points raised to discuss below. Feel free to expand and correct these, and to add appropriate labels to the PR so that other people can chime in too 😊
So this will go under
I think we can keep both fields. But note that the output of the two fields as observed below is different.
Agreed that metadata should be it's own key, with the metadata dictionary under it.
And in case of modern editable installs, the location can be derived by reading Also the updated So do we also want to change that in the default format as well, or keep it untouched? |
a0a0a5f
to
e49a3ad
Compare
Hi @sbidoul I have gone ahead and added your suggestions made at #8008 (comment) to this PR. Could you please look at them and add your suggestions if anything else is needed, and accordingly I can close the work on the docs here, and start with the actual implementation. |
898710e
to
29c7395
Compare
docs/html/reference/pip_show.rst
Outdated
|
||
*requested* | ||
A boolean, set to True if ``REQUESTED`` is present, | ||
as specified in :pep:`376`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should provide the content of REQUESTED
as for INSTALLER
. This would allow for a future evolution of pep 376 where, e.g., REQUESTED would contain a version specifier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 02759a3
docs/html/reference/pip_show.rst
Outdated
*location* | ||
A string containing the path where the distribution is installed. For legacy installs, | ||
this is the parent directory of the metadata (.dist-info or .egg-info) directory. | ||
For legacy editable installs, this is the directory where the source is located. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be simplified by saying that location is the parent of the metadata (dist-info or egg-info) directory. This holds True for all kinds of installation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in 02759a3
docs/html/reference/pip_show.rst
Outdated
*required_by* | ||
A list of canonicalized distribution names that depend | ||
on the queried distribution. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add requires
: a list of canonicalized distribution names which this distribution depends on.
Although we'd need to think about how it would work exactly with extras and environment markers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add
requires
: a list of canonicalized distribution names which this distribution depends on.
Added in 02759a3
Although we'd need to think about how it would work exactly with extras and environment markers.
Just to confirm, I assume that you are mentioning to PEP-508 extras for extras
and PEP-496 for environment markers
It would also be helpful if you could take an example of a package, and perhaps outline what would be the expected output json in case of both, and then I can take that and the info from above links to think of a solution.
Another thing I can think of is to get out the basic version of pip show --format=json
with what we have right now, and then think about these later. (if these are considered as advanced use cases of show by us, and can be targeted in another PR)
@deveshks thanks for your work on this. I'd like to have more opinions from other pip maintainers before going forward. We also need to check if the proposed json structure is applicable to |
Agreed on that, but what would be a good way of getting that opinion?
IIUC, Also |
As per the comment #8008 (comment) , it would be great if the other maintainers can take a look at this PR and share their thoughts, in order to move this forward. |
02759a3
to
6f68007
Compare
Hi @pradyunsg , @pfmoore , @uranusjr As per @sbidoul 's comment #8008 (comment) , I would love to have your opinions on his concerns and have them addressed, before moving this PR forward. |
I'm still on board with Stephane's suggestions with the same question on the (partial) duplication between a possible |
Thanks a lot for the response @xavfernandez. I believe that to being with, we can keep both field and clearly document where does those fields differ. I have tried to outline that and the 4th point which are the unresolved ones in #8008 (comment) in #8008 (comment) . I would be glad if you could take a look at them and provide suggestions as well. |
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
430f3f2
to
e3bc7fb
Compare
Hello! I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the |
@deveshks Would you be interested in updating this PR, to resolve the merge conflicts? |
@pradyunsg yes, I will update this PR and will resolve merge conflicts in a day's time, and accordingly work to move this PR forward. |
A gentle nudge. :) |
@pradyunsg I think this PR can be kept as a placeholder to discuss the extended json output for pip list and pip show, but the implementation of that format is still TODO, and this PR must not be merged before the implementation. |
64b992e
to
6ccc19e
Compare
6ccc19e
to
aa4b183
Compare
Hi @pradyunsg , apologies for the delay, I have resolved the merge conflicts and updated the PR. Please let me know how we can proceed further on this. |
Towards #5261
As per #7967 (comment) , this serves as a companion PR for #7967 by providing explanation of different fields for
pip show
in both header and json format.cc @pradyunsg @sbidoul @xavfernandez